Make push_error and push_warning print name of calling function
#93648
Conversation
|
Could you provide screenshots on how this would look now? Also check out the static check errors. |
|
Also I'm not sure this change makes sense, this method is called from scripting, the GDScript method won't be shown, so please add some screenshots to see what this outputs We don't want it to print the c++ method that called into scripting IMO |
|
Okay, I have fixed the formatting and code style issues, moved the platform dependent code into relevant subclasses of To clarify, there are two cases this fix covers:
Current behaviorRuntime GDScriptAs I said, this is partially implemented already, but still includes an unhelpful reference to 
Editor GDScriptGDScript run in the editor did not properly show where it was called from before, either in the editor logger or to stdout. Here is the view from the editor: 
and here is the view from stdout (with Rider): 
C++Here is a warning generated from 
and the view from stdout: 
Proposed improvementsRuntime GDScript
Editor GDScriptView in editor logger: View in stdout: 
C++View in editor logger: 
View in stdout: 
There are I'm sure still ways to make the results look a little better. All of the various logger implementations seem to have different ideas of what the two different string arguments to |
|
Would #91006 be of any use here? |
Yes, that looks like it would do very much the same thing as what I do for GDScript stacktraces. Though theirs is a much more sweeping change, and mine is much more targeted. Mine requires being run in debug mode to get an accurate GDScript backtraces, while theirs completely rewrites GDScript's stack system to be fast enough to run all the time. There are pros and cons to both approaches. One thing my PR does that theirs doesn't is print the C++ stacktrace, which is still needed if |
|
|
||
| #if !defined(__ANDROID__) && !defined(__EMSCRIPTEN__) | ||
| virtual Vector<StackInfo> get_cpp_stack_info() const override; | ||
| #endif |
There was a problem hiding this comment.
These should be in the respective OS implementations I'd say, i.e. Mac and Linux, or overridden as stubs in Android and Web, this makes it hard to track
There was a problem hiding this comment.
@AThousandShips I think it makes the most sense to keep them in OS_Unix because execinfo.h is theoretically a part of any standards-compliant Unix system. The problem is that Android and Emscripten aren't fully POSIX compliant and don't provide the execinfo.h header. I was surprised to even see that they were children of OS_Unix because neither are really Unix systems. I can remove the #ifdefs from the header here, but I will need to keep them in os_unix.cpp (which already has a ton of these because of the differences in different Unix implementations) because the implementation there depends on being able to import execinfo.h, which is not present in either
There was a problem hiding this comment.
Fair enough, but this makes it harder to find as it'd be expected to be in those platforms instead of drivers
There was a problem hiding this comment.
Yeah, it's weird to me that OS_MacOS in platform inherits from OS_Unix in drivers, which inherits from OS in core, and each provides different portions of the platform class's implementation, but here we are.
|
Precisely these days I'm trying to hunt down the root trigger of certain operations and the ability to obtain a proper C++ stack trace would come super handy. Regarding the implementation, I'd just like to point out that the various |
This reverts commit 029246d.
Incorporate suggestions from @AThousandShips and necessary code changes to make those work.
@RandomShaper Yes, just looking at the MacOS implementation, there is definitely a lot of overlap in functionality that could be moved into a separate library to save some duplication. If you would like to contribute a refactor to do that, you are more than welcome to. Personally, I am happy with this version and need to move on to other projects. |
| #ifdef DEBUG_ENABLED | ||
| String OS_MacOS::get_debug_descriptor(const StackInfo &p_info) { | ||
| String pipe; | ||
| Error error = execute("atos", { "-o", p_info.file, "-l", String::num_uint64(reinterpret_cast<uint64_t>(p_info.load_address), 16), String::num_uint64(reinterpret_cast<uint64_t>(p_info.symbol_address), 16) }, &pipe); |
There was a problem hiding this comment.
atos call probably should have -arch arm64 or -arch x86_64 arg added based on the running executable architecture, or in some conditions (like running under Rosetta) it might try reading wrong architecture from the fat binary.
Would it be fine with you that someone (possibly me when I can spend time on this) takes over your PR? That would mean that other contributor would take your changes, apply the refactor and commit them to a new PR superseding this one, crediting both authors (by having separate commits if it makes sense for the repo or using a co-author annotation). |
Yes, that would be fine. Just give me credit in some way. |
Currently,
push_errorandpush_warninggive themselves as the callingfunction, which is not useful for debugging. I have altered these to use the
C stacktrace to get the name of the function that called them. This saves
having to turn these functions into macros, which would require recompiling
every single piece of code that uses them. I have also set it to, when debug
symbols are available, use them with
atos(available on macOS) to find theexact source file and line being called from. It should be possible to the same
thing on Linux using
addr2line, but I don't have a Linux box handy to testthat. I am not sure how you would implement this in Windows.
I have tested this on macOS Sonoma 14.5 (23F79). It should work on any *nix
system. I am not sure about Windows.
Fixes #76770